-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Ref script size fee calculation + Ref input support (tx builder) #349
Conversation
Fee calculation has changed to take into account ref script sizes via the new protocol parameter `min_fee_ref_script_cost_per_byte`.
@@ -227,10 +228,54 @@ fn min_fee(tx_builder: &TransactionBuilder) -> Result<Coin, TxBuilderError> { | |||
fn min_fee_with_exunits(tx_builder: &TransactionBuilder) -> Result<Coin, TxBuilderError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_fee might possibly need updating I think, or even rethinking these two because we have more components to the fee calculations now - but maybe not since can you even possibly incur this ref script fee if you don't have exunits involved? but it could be involved just give temporarily incorrect values (e.g. for seeing how much doing something changes fee mid-build before plutus contract is added)
pub fn min_ref_script_fee( | ||
tx: &Transaction, | ||
linear_fee: &LinearFee, | ||
total_ref_script_size: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't possible to know the original sizes so we can't compute this from just the Transaction
anymore
// } | ||
// } | ||
|
||
let ref_script_orig_sizes: HashMap<ScriptHash, u64> = if let Some(ref_inputs) = &tx_builder.reference_inputs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading CIP33 it says:
The key idea is to use reference inputs and modified outputs which carry actual scripts ("reference scripts"), and allow such reference scripts to satisfy the script witnessing requirement for a transaction. This means that the transaction which uses the script will not need to provide it at all, so long as it referenced an output which contained the script.
so I'm going on the assumption that if the hash matches something created as a reference script in a reference input's output that it must be a reference script and will incur this extra fee
other option: be explicit during tx builder/input builder and get the info from there
// https://github.com/IntersectMBO/cardano-ledger/blob/7e65f0365eef647b9415e3fe9b3c35561761a3d5/eras/conway/impl/src/Cardano/Ledger/Conway/Tx.hs#L84 | ||
// https://github.com/IntersectMBO/cardano-ledger/blob/a34f878c56763d138d2203d8ba84b3af64d94fce/eras/conway/impl/src/Cardano/Ledger/Conway/UTxO.hs#L152 | ||
use num::{rational::Ratio, CheckedAdd, CheckedMul}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might need Ratio<BigNum>
during calculations then conversion afterwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but do we care about other usage of the fraction crate? I changed this here since it just panics even though the final operation could fall into a u64
fine if the intermediate denom/numerators don't. The other place we use the fraction crate is in genesis parsing.
let mut total_ref_script_size = 0; | ||
for utxo in tx_builder.inputs.iter() { | ||
if let Some(Credential::Script { hash, .. }) = utxo.output.address().payment_cred() { | ||
println!(" ------ searching: {}", hash.to_hex()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we want to remove all these print statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Sorry, I forgot to set it back to draft. I just wanted to check mergability.
@@ -402,6 +463,7 @@ pub struct TransactionBuilder { | |||
utxos: Vec<InputBuilderResult>, | |||
collateral_return: Option<TransactionOutput>, | |||
reference_inputs: Option<Vec<TransactionUnspentOutput>>, | |||
//reference_scripts_used: Vec<Script>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this?
PlutusScriptWitness::Ref(ref_script) => { | ||
PlutusScriptWitness::Ref(ref_script_hash) => { | ||
// it could also be a reference script - check those too | ||
// TODO: do we want to change how we store ref scripts to cache the hash to avoid re-hashing (slow on wasm) every time an input is added? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO? Okay to merge with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, yes. I don't think it's too bad of a performance hit for now (did not profile it - just going off past experience with hashing in the builder and we don't support asm.js in the new CML anyway so it's not as ridiculous a difference) and it's only when using ref scripts and we can always optimize later, using this comment as a reminder.
Fee calculation has changed to take into account ref script sizes via the new protocol parameter
min_fee_ref_script_cost_per_byte
.TxBuilder has been updated to actually take into account reference inputs. Before it let you add them to transactions but the witness checks were oblivious to them.
Draft PR still. I have never worked with reference inputs or reference scripts before so some of this could be incorrect still